Conversation
|
LGTM so far |
bmarty
left a comment
bmarty
left a comment
There was a problem hiding this comment.
Thanks for your work.
Some first small remarks, will test it later.
| views.composerLayout.collapse() | ||
| views.composerLayout.setTextIfDifferent(text) | ||
| views.composerLayout.views.sendButton.contentDescription = getString(R.string.send) | ||
| private fun renderRegularMode(content: String, messageType: String) { |
There was a problem hiding this comment.
The body content does not match the name of this fun anymore.
Is it possible to not change this fun and create a new one like renderVoiceMessageMode()?
And test the value of messageType at the call site.
| // We should improve the UX to support going into playback mode when paused and delete the media when the view is destroyed. | ||
| roomDetailViewModel.handle(RoomDetailAction.EndAllVoiceActions(deleteRecord = false)) | ||
| views.voiceMessageRecorderView.initVoiceRecordingViews() | ||
| roomDetailViewModel.handle( |
There was a problem hiding this comment.
We do not talk to the textComposerViewModel anymore here?
There was a problem hiding this comment.
roomDetailViewModel will decide it and communicate with textComposerViewModel later with SaveDraft event.
| views.voiceMessageRecorderView.initVoiceRecordingViews() | ||
| roomDetailViewModel.handle( | ||
| RoomDetailAction.OnRoomDetailEntersBackground( | ||
| isVoiceMessageActive = views.voiceMessageRecorderView.isActive() |
There was a problem hiding this comment.
It is strange to ask the view about the state. The ViewModel should be aware of the current state.
There was a problem hiding this comment.
I found it complex to determine the state here, especially if there is already a draft in the room.
| override fun startRecord() { | ||
| init() | ||
| outputFile = File(outputDirectory, "Voice message.$filenameExt") | ||
| val fileName = """${UUID.randomUUID()}.$filenameExt""" |
There was a problem hiding this comment.
The filename can be visible to the user, it will be a bit weird.
Is it possible to create a folder per room?
Also saving voice message draft could be handled by the Matrix SDK, using the DraftService
There was a problem hiding this comment.
I proposed a way to keep drafts in a roomId based directory structure.
|
CI is not happy, can you handle that please? Thanks |
| ?.transform { | ||
| it.setString(DraftEntityFields.MESSAGE_TYPE, MessageType.MSGTYPE_TEXT) | ||
| } | ||
|
|
There was a problem hiding this comment.
This will be a problem for those who are already on version 19. In this case you should create a version 20.
bmarty
left a comment
bmarty
left a comment
There was a problem hiding this comment.
I'm sorry I think this has to be modified.
I would like the voice messages to be saved as draft by the SDK, using DraftService.
It will simplify the code I think and make it more robust, by following our current architecture.
What do you think about it?
|
@bmarty from my understanding we are storing the draft via the I'm unfamiliar with the area, do you have more insight into the type of change you're expecting? |
| val audioJsonString = audioType?.toContentAttachmentData()?.toJsonString() | ||
| _viewEvents.post(RoomDetailViewEvents.SaveDraft(audioJsonString, MessageType.MSGTYPE_AUDIO)) | ||
| } else { | ||
| _viewEvents.post(RoomDetailViewEvents.SaveDraft(null, MessageType.MSGTYPE_TEXT)) |
|
going to close this PR whilst working on it |
3 participants
If a user leaves the room detail screen without sending or deleting the ongoing voice message record then we need to save this record as a draft message and render it again in playback mode when the user enters the room detail screen again.
EDIT
Fixes #3922